Broken title search ut #411
Conversation
dosubot
bot
added
size:S
tests/utils/paper_helpers.py
Outdated
| citation_pattern = r"(This article has )\d+( citations?)" | ||
|
|
||
| # between group 1 and 2, replace with the character "n" | ||
| expected_cleaned = re.sub(citation_pattern, r"\1n\2", expected).strip() | ||
| actual_cleaned = re.sub(citation_pattern, r"\1n\2", actual).strip() |
There was a problem hiding this comment.
I think you got the capturing groups wrong, you want r"This article has (\d+) citations?"
We don't care to extract the text, we want to extract the number
Then you can just directly compare the int, no need for the weird r"\1n\2"
There was a problem hiding this comment.
That's fair.
This original approach using \1n\2 was because it's focusing on comparing the structure of the citation text while ignoring the citation count itself; just ignoring whatever number is there.
But you make a valid point. I'll get this updated!
There was a problem hiding this comment.
Fwiw (I am not sure your experience levels with regex) going from r"(This article has )\d+( citations?)" to r"This article has (\d+) citations?" will still match the surrounding text too. A capturing group () is here to make it easy to capture and extract a value
And plz point out if we're on the same page already haha. Hope you have a good night
There was a problem hiding this comment.
I believe I've addressed this accordingly. Thx James!
tests/utils/paper_helpers.py
Outdated
| :return: True if the citations match except for the citation count, False otherwise. | ||
| """ | ||
| # https://regex101.com/r/lCN8ET/1 | ||
| citation_pattern = r"(This article has )\d+( citations?)" |
There was a problem hiding this comment.
This regex is closely coupled to DocDetails. Can you move this regex to a ClassVar[str] on DocDetails?
dosubot
bot
added
size:M
|
The minor variations, i.e. 196 vs 191 is expected, because the papers will get more citations over time. The bigger question is why the cassettes aren’t being used because this should come back with a recorded request each time, I’m in favor of root causing that before we merge this. Maybe we regenerated these cassettes but didn’t update the test? |
tests/test_clients.py
Outdated
| assert ( | ||
| expected_citation_format is not None | ||
| ), "Expected string should match the citation pattern" | ||
| assert ( | ||
| actual_citation_format is not None | ||
| ), "Actual string should match the citation pattern" |
There was a problem hiding this comment.
| assert ( | |
| expected_citation_format is not None | |
| ), "Expected string should match the citation pattern" | |
| assert ( | |
| actual_citation_format is not None | |
| ), "Actual string should match the citation pattern" | |
| assert ( | |
| expected_citation_format | |
| ), "Expected string should match the citation pattern" | |
| assert ( | |
| actual_citation_format | |
| ), "Actual string should match the citation pattern" |
This may resolve the union-attr ignore a few lines below
tests/test_clients.py
Outdated
| expected_remaining = ( | ||
| paper_attributes["formatted_citation"][: expected_citation_format.start()] | ||
| + paper_attributes["formatted_citation"][expected_citation_format.end() :] | ||
| ) | ||
|
|
||
| actual_remaining = ( | ||
| details.formatted_citation[: actual_citation_format.start()] # type: ignore[union-attr] | ||
| + details.formatted_citation[actual_citation_format.end() :] # type: ignore[union-attr] | ||
| ) | ||
|
|
||
| # Assert that the parts of the strings outside the citation count are identical | ||
| assert ( | ||
| expected_remaining == actual_remaining | ||
| ), "Formatted citation text should match except for citation count" |
There was a problem hiding this comment.
So we are doing a regex search to find a region, then removing that region, and lastly doing an equality check on the front/back end.
I think it would be simpler to do one regex removal of the region, and directly equality compare the remanants.
I also think this is what you originally had 😅 😓 hahaha sorry! I actually realized I misunderstood your original logic last night, my bad here.
What do you think?
There was a problem hiding this comment.
definitely! no worries, communicating through PRs can be hard sometimes. Thank you!! adjusting now.
before I merge this I'm also going to look into why the cassette file aren't (hasn't) been updated
dosubot
bot
added
size:S
dosubot
bot
added
size:XL
dosubot
bot
removed
the
size:XL
dosubot
bot
added
the
size:L
Co-authored-by: James Braza <[email protected]>
Summary of Problem
The expected vs asserted citation count was not matching, causing this UT to specifically fail.
I noticed if I switched the order of sources from
semantic_scholar, crossreftocrossref, semantic_scholarthe citation count would be even more dramatically different (similar to the results James was noticing earlier)More importantly, I found the papers on semantic scholar (25); which doesn't match our expected value (23); so that failure makes sense.
The following assertion is failing for the same reason: 196 assertion vs 191 expected.
Vibe checked the results of this change via a CI run: https://github.com/Future-House/paper-qa/actions/runs/10859898582/job/30139826477
This does not fix the issue that James found earlier this evening in test_agents.py.